Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid deadlock during first time initialisation #657

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mac-cain13
Copy link

Summary

Since the UserDefaults calls made by saveIdentity cause blocking NSNotifications to be emitted to the main thread, calling saveIdentity under the write lock can cause deadlocks.

Since the saveIdentity call only reads state it’s now moved to only take a read lock instead of being under the write lock.

The problem

In my Mac app I initialise Mixpanel on a background thread to avoid blocking the main thread during launch. This works fine for most users, however when the app loses focusses immediately during the very first launch Mixpanel deadlocks. Freezing the app completely. (This happens most often when users install the app through the App Store en then click the "Open" button in the App Store itself, this launches the app, but als takes back the focus immediately.)

Attached here you can find the stacktrace where the app hangs: MixpanelHangStackTrace.txt

The cause

Turns out that the MixpanelInstance.unarchive method calls MixpanelPersistence.saveIdentity while inside ReadWriteLock.write. This in turn writes to UserDefaults which blocks until it's NSNotification for the defaults updates are picked up by the runloop on the main thread.

However at the moment that unarchive is doing its thing, the main thread is busy handing the NSApplication.willResignActiveNotification this wants to check the opt-out state and therefore is waiting for the ReadWriteLock to become available again. Causing a deadlock.

Solution

Since strictly all state writing is already done by the time saveIdentity is called, I think the most robust solution is to move the saveIdentity out of the write lock. It does need a read lock, because it does read the internal state to store it in UserDefaults, but writing isn't needed. Calling saveIdentity is also done without a read lock in other places in the codebase.

I made the necessary adjustments in this PR and verified this solves the deadlock in my case.

Note on reproduction

I can quite easily reproduce this in my own app by putting a breakpoint in the unarchive method on the line setting the anonymousId. Hitting the breakpoint makes my app resign focus (because Xcode takes focus) while still under the write lock.

However when I tried to make a reproduction sample I found it hard to make a simple demo. Since it only happens at first launch and I need the breakpoint to trigger it reliably.

Since the UserDefaults calls made by saveIdentity cause blocking NSNotifications to be emitted to the main thread, calling saveIdentity under the write lock can cause deadlocks.

Since the saveIdentity call only reads state it’s now moved to only take a read lock instead of being under the write lock.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant